-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-29453][WEBUI] Improve tooltips information for SQL tab. #26259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@dongjoon-hyun , @shahidki31 , @srowen (All the comments given in this PR #26216(closed) i have fixed and raised new PR). |
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala
Outdated
Show resolved
Hide resolved
|
Jenkins, test this please |
srowen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this adds enough to justify. There just isn't much to explain about these.
(PS normally you push more commits to your previous branch rather than make a new one and PR, so as not to fork the review)
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala
Outdated
Show resolved
Hide resolved
|
|
If we can't come up with meaningful tooltips, why add them? I don't know if it is necessary here. |
|
Yep I think those tooltips are not useful and should be removed. |
ok, i will remove the tooltips. @srowen , i think this 3 is ok, please check and others i have removed. |
### What changes were proposed in this pull request?
Adding tooltip to SQL tab for better usability.
https://issues.apache.org/jira/browse/SPARK-29453
### Why are the changes needed?
There are a few common points of confusion in the UI that could be clarified with tooltips. We should add tooltips to explain.
### Does this PR introduce any user-facing change?
yes.
### How was this patch tested?
1) Unit tests (written unit test cases to verify changes).
2) Manual test.
Authored-by: Ankit Raj Boudh <[email protected]>
fixed review comments
|
@dongjoon-hyun ,@srowen, I have fixed all the comments please check it. I think now every think is ok. |
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala
Outdated
Show resolved
Hide resolved
Tooltips not required for each row review comments fixed.
|
@shahidki31 and @srowen now I think fixed is ok? |
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala
Show resolved
Hide resolved
srowen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't think this adds enough to justify more tooltips popping up. It isn't required to have every item with a tooltip.
|
@srowen, if headerTooltip is empty then no tooltips popup will display. |
|
To be clear, I would not merge this. |
|
Ok then I need add check point, if header is empty then no need to add tooltips, then it's ok? |
|
#26259 (comment) |
|
@srowen , so this issue not require to fix ? , (if yes then i will commit in jira not require to fix, so that reporter will cancel that jira) |
|
As I say here and elsewhere, I don't think most of these columns have anything meaningful to say in a tooltop. Duration, maybe, if it's not clear what the end and start times are. But no I do not think we need most of what's being proposed here. |
|
ok then for Duration i will add the tooltip for other no need to add that is ok @srowen ? |
|
As long as it's not saying "the time taken to X", maybe so. You'd have to figure out how to correctly describe the start and end time it's measuring. |
|
ok i will check this point (how to correctly describe the start and end time it's measuring.) |
|
@srowen , this is how we are calculating duration and i referred this PR [SPARK-29019][WebUI] Improve tooltip JDBC/ODBC Server tab #25723 Tooltips for "Duration": "Difference between sql query execution finish time and submission time". If this suggestion ok then i will submit PR for this |
|
I don't think that's quite accurate. It's either the time until the finish time, or the current time if still executing. How about more like "Time from query submission to completion (or if still executing, time since submission)". You/we might also review other similar tooltips about duration to see if we can make the wording more standard, but something like that. |
|
#26259 (comment) @srowen , sure i will check other places also. |
|
@srowen, I checked the code and found Total 5 pages of Spark showing Duration field inside table , Please check all the screenshot :
Note : Active Driver and Completed Driver under master spark ui page also "Duration field is there." |
|
@srowen , I think based on feature "Duration" field tooltips will change. |








What changes were proposed in this pull request?
Adding tooltip to SQL tab for better usability.
Why are the changes needed?
There are a few common points of confusion in the UI that could be clarified with tooltips. We
should add tooltips to explain.
Does this PR introduce any user-facing change?
yes.

How was this patch tested?
Manual test.